Conversation
There was a problem hiding this comment.
Review: Move agent rules to ~/.claude/CLAUDE.md
The approach is sound — ~/.claude/CLAUDE.md is the correct user-level global config location for Claude Code, and this cleanly fixes the CWD dependency introduced by PR #882. The symlink hack removal is a good simplification.
Issues
1. test_no_rules_does_nothing not updated (tests/sandbox/test_entrypoint.py:706-720)
This test still patches os.lchown (line 707) even though the production code no longer calls it. It also doesn't set config.claude_dir on the mock. It works today because the function returns early before reaching claude_dir, but it's stale — the patch decorator is misleading and the mock is incomplete.
Suggested fix: remove the @patch("os.lchown") decorator and its mock_lchown parameter, and add config.claude_dir = temp_dir / ".claude" to the mock setup for consistency with the other tests.
This is minor and non-blocking.
2. sandbox/.claude/rules/README.md not updated (lines 10, 12)
The README still says:
- `~/CLAUDE.md` → All rules combined
and:
Combining all rules into `~/CLAUDE.md` ensures they're always available.
These should be updated to reference ~/.claude/CLAUDE.md to match the new location and avoid confusing future readers.
Non-blocking.
3. No cleanup of stale ~/CLAUDE.md from previous runs
On persistent volumes or bind-mounted home directories, upgrading from the old behavior leaves a stale ~/CLAUDE.md behind. Claude Code may pick it up as a project-level config in addition to the global ~/.claude/CLAUDE.md, resulting in duplicate rules. Similarly, the old ~/repos/CLAUDE.md symlink (now pointing to a nonexistent target) could cause warnings.
This is worth considering — a one-time cleanup in setup_agent_rules that removes config.user_home / "CLAUDE.md" and config.repos_dir / "CLAUDE.md" if they exist would prevent this. Not blocking since fresh containers won't have this issue, but if existing volumes are reused it could cause confusing behavior.
Correctness verified
config.claude_diris a well-established property (entrypoint.py:249) returning~/.claude/.- Call order is
setup_agent_rules→setup_claude, so themkdir(parents=True, exist_ok=True)at line 825 correctly creates the directory before writing. The duplicatemkdirat line 846 insetup_claudeis harmless (exist_ok=True). setup_claudedoes not touchCLAUDE.md— it writessettings.json,.claude.json, and copies commands. The file written bysetup_agent_rulesis preserved.chown_recursiveat line 971 covers CLAUDE.md along with the rest of~/.claude/, so ownership is correct regardless of the individualos.chownat line 827.- The symlink removal and
os.lchownremoval are clean — no other code references the old symlink path.
Verdict
Approve. The core change is correct and well-scoped. The three items above are all non-blocking — the README staleness and the missing test cleanup are easy follow-ups.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Fix test_no_rules_does_nothing: remove stale os.lchown patch, add config.claude_dir to mock setup for consistency with other tests. - Update sandbox/.claude/rules/README.md to reference ~/.claude/CLAUDE.md instead of the old ~/CLAUDE.md location. - Add cleanup of stale ~/CLAUDE.md and ~/repos/CLAUDE.md symlink in setup_agent_rules to prevent duplicate rules on persistent volumes.
There was a problem hiding this comment.
All three items from the review have been addressed in 4541a77:
-
test_no_rules_does_nothing— Removed the stale@patch("os.lchown")decorator and itsmock_lchownparameter. Addedconfig.claude_dir = temp_dir / ".claude"to the mock setup for consistency with the other tests. -
sandbox/.claude/rules/README.md— Updated references from~/CLAUDE.mdto~/.claude/CLAUDE.md. -
Stale file cleanup — Added cleanup in
setup_agent_rulesthat removes~/CLAUDE.mdand~/repos/CLAUDE.md(if they exist or are dangling symlinks) after writing the new file. This prevents duplicate rules on persistent volumes.
Tests pass (4/4 in TestSetupAgentRules), linter clean.
— Authored by egg
There was a problem hiding this comment.
No agent-mode design concerns. This is a straightforward infrastructure fix that moves agent rules to the documented Claude Code global config location (~/.claude/CLAUDE.md), removing the symlink hack. No impact on agent autonomy, prompt design, or workflow patterns.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: Move agent rules to ~/.claude/CLAUDE.md
All three items from the previous review are properly addressed in 4541a77:
1. test_no_rules_does_nothing (tests/sandbox/test_entrypoint.py) — Fixed. The stale @patch("os.lchown") decorator and mock_lchown parameter are removed. config.claude_dir is now set on the mock for consistency with the other tests.
2. sandbox/.claude/rules/README.md — Fixed. Both references updated to ~/.claude/CLAUDE.md, and the "regardless of CWD" clarification is a good addition.
3. Stale file cleanup (sandbox/entrypoint.py:829-835) — Fixed. The exists() or is_symlink() pattern correctly handles both regular files and dangling symlinks. The cleanup runs after the new file is written, so the upgrade path is safe. Running as root means permission failures aren't a concern.
No new issues found. The delta is clean and minimal — exactly what was suggested, nothing more.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
|
egg feedback addressed. View run logs 4 previous review(s) hidden. |
Summary
PR #882 changed the container startup directory from
~/repos/to~/repos/egg/(the detected single git repo). This broke the CLAUDE.md pickup because the agent
rules were written to
~/CLAUDE.mdwith a symlink at~/repos/CLAUDE.md— neitherof which is in the new CWD.
This fix moves the agent rules to
~/.claude/CLAUDE.md, which is Claude Code'sdocumented user-level global config location. It is loaded automatically in every
session regardless of working directory, eliminating the dependency on CWD and
removing the need for the
~/repos/CLAUDE.mdsymlink hack.Issue: none
Test plan:
pytest tests/sandbox/test_entrypoint.py— all 89 tests pass~/repos/egg/(single repo) or~/repos/(multiple repos)Authored-by: egg